Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add process control plug #14

Merged
merged 6 commits into from
Oct 16, 2023

Conversation

MonicaisHer
Copy link
Contributor

@MonicaisHer MonicaisHer commented Oct 11, 2023

This PR will resolve #8. As the issue described, there is a seccomp violation on a sched_setattr system call, and snappy-debug suggests adding the process-control plug.

chip-tool toggle command success rates :

process-control plug without with
chip-tool toggle command success rates 8/10 (2 Delay)
9/10 (1 Timeout)
8/10 (2 Timeout)
9/10(1 Delay)
9/10 (1 Timeout)
10/10
7/10 (3 Timeout)
10/10
10/10
9/10 (1 Timeout)

Note: Timeout is a fatal error; Delay is not a fatal error.

Overall, using a process-control plug provides chip-tool snap with slightly higher success rates when controlling a device.

Copy link
Member

@farshidtz farshidtz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR addresses two unrelated issues, one for documenting connections on UC, and another for changing the packaging to allow granting access to a new interface (which itself isn't being documented).

@@ -127,6 +127,7 @@ apps:
- network
- bluez
- avahi-observe
- process-control
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is process-control interface being added? As issue #8 shows, snappy-debug suggests this interface because there is a seccomp violation on a sched_setattr system call. Is this affecting the functionality?

See also https://snapcraft.io/docs/debug-snaps#heading--seccomp

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it does affect the functionality. Please see the PR description has been updated to include an explanation.

The solution, which involves running the scmp_sys_resolver command to solve the Seccomp violation, appears to be less user-friendly compareing to simply adding the process-control plug.

Copy link
Member

@farshidtz farshidtz Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it does affect the functionality. Please see the PR description has been updated to include an explanation.

The solution, which involves running the scmp_sys_resolver command to solve the Seccomp violation, appears to be less user-friendly compareing to simply adding the process-control plug.

The data you shared shows the same success rate (4/50 timeouts) with and without connecting the interfaces. The delays may be caused by external networking and computing factors; it appears that the tests have been done in batches without randomization.

I'm not sure what solution are you referring to, involving scmp_sys_resolver. The shared document uses that command to resolve the syscall name from its ID.

@MonicaisHer MonicaisHer changed the title Add process control plug, update snap connection commands Add process control plug Oct 12, 2023
@MonicaisHer MonicaisHer marked this pull request as draft October 12, 2023 12:56
@MonicaisHer MonicaisHer marked this pull request as ready for review October 12, 2023 15:34
README.md Outdated Show resolved Hide resolved
@MonicaisHer MonicaisHer force-pushed the add-connection-and-readme branch from 9569295 to d2b06c7 Compare October 16, 2023 08:41
@MonicaisHer MonicaisHer requested a review from farshidtz October 16, 2023 08:43
@farshidtz farshidtz merged commit 8ca5ae2 into canonical:main Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Seccomp violation when pairing and controlling devices
2 participants